Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@llvm/pr-subscribers-mlir-core

Author: Lin-Ya Yu (yu810226)

diff --git a/mlir/include/mlir/IR/Properties.td b/mlir/include/mlir/IR/Properties.td
index a6221f9aaaef9..a7ade0675b9bb 100644
--- a/mlir/include/mlir/IR/Properties.td
+++ b/mlir/include/mlir/IR/Properties.td
@@ -773,9 +773,10 @@ class OptionalProp<Property p, bit canDelegateParsing = 1>
   }];
   let writeToMlirBytecode = [{
     $_writer.writeOwnedBool($_storage.has_value());
-    if (!$_storage.has_value())
-      return;
-  }] # !subst("$_storage", "(*($_storage))", p.writeToMlirBytecode);
+    if ($_storage.has_value()) {
+      }] # !subst("$_storage", "(*($_storage))", p.writeToMlirBytecode) # [{
+    }
+  }];
 
   let hashProperty = !if(!empty(p.hashProperty), p.hashProperty,
     [{ hash_value($_storage.has_value() ? std::optional<::llvm::hash_code>{}] #

@llvm/pr-subscribers-mlir-ods

Author: Lin-Ya Yu (yu810226)

diff --git a/mlir/include/mlir/IR/Properties.td b/mlir/include/mlir/IR/Properties.td
index a6221f9aaaef9..a7ade0675b9bb 100644
--- a/mlir/include/mlir/IR/Properties.td
+++ b/mlir/include/mlir/IR/Properties.td
@@ -773,9 +773,10 @@ class OptionalProp<Property p, bit canDelegateParsing = 1>
   }];
   let writeToMlirBytecode = [{
     $_writer.writeOwnedBool($_storage.has_value());
-    if (!$_storage.has_value())
-      return;
-  }] # !subst("$_storage", "(*($_storage))", p.writeToMlirBytecode);
+    if ($_storage.has_value()) {
+      }] # !subst("$_storage", "(*($_storage))", p.writeToMlirBytecode) # [{
+    }
+  }];
 
   let hashProperty = !if(!empty(p.hashProperty), p.hashProperty,
     [{ hash_value($_storage.has_value() ? std::optional<::llvm::hash_code>{}] #

@llvm/pr-subscribers-mlir

Author: Lin-Ya Yu (yu810226)

diff --git a/mlir/include/mlir/IR/Properties.td b/mlir/include/mlir/IR/Properties.td
index a6221f9aaaef9..a7ade0675b9bb 100644
--- a/mlir/include/mlir/IR/Properties.td
+++ b/mlir/include/mlir/IR/Properties.td
@@ -773,9 +773,10 @@ class OptionalProp<Property p, bit canDelegateParsing = 1>
   }];
   let writeToMlirBytecode = [{
     $_writer.writeOwnedBool($_storage.has_value());
-    if (!$_storage.has_value())
-      return;
-  }] # !subst("$_storage", "(*($_storage))", p.writeToMlirBytecode);
+    if ($_storage.has_value()) {
+      }] # !subst("$_storage", "(*($_storage))", p.writeToMlirBytecode) # [{
+    }
+  }];
 
   let hashProperty = !if(!empty(p.hashProperty), p.hashProperty,
     [{ hash_value($_storage.has_value() ? std::optional<::llvm::hash_code>{}] #

@fabianmcg as the author of the original commit

The only issue I can raise is,this is untested in this patch (I'm not familiar with back port requirements on tests, so it'd be great if someone can comment on that). In the other PR, the issue was indirectly tested by the ptr op.

Thanks for the review!

This is a clean backport of the fix from main. As pointed out, the issue is indirectly tested in the original commit (via the ptr op), so I didn’t backport the tests here.
If anyone familiar with the backport testing process could comment on the test coverage question (raised above) or help with merging, that’d be much appreciated. I don’t have write access myself.

Sorry we did not see this sooner as it was not added to the release milestone, so it did not show up our query of open backport requests.

The bug that this is fixing, when was the bug introduced? Since this is the very last release from the 21.x branch, we generally only want to accept fixes for regressions or small fixes that have a wide impact. As a consequence of this being the last release from the 21.x branch, if there are any issues that may arise from this change, we would not be able to fix it until the LLVM 22 release.

Given that, @fabianmcg, do you feel this change is very low risk and a fix for a recent regression that would merit including in 21.x instead of just waiting for LLVM 22?

ping @fabianmcg

The bug that this is fixing, when was the bug introduced? Since this is the very last release from the 21.x branch, we generally only want to accept fixes for regressions or small fixes that have a wide impact. As a consequence of this being the last release from the 21.x branch, if there are any issues that may arise from this change, we would not be able to fix it until the LLVM 22 release.

Given that, @fabianmcg, do you feel this change is very low risk and a fix for a recent regression that would merit including in 21.x instead of just waiting for LLVM 22?

Apologies, I missed the ping

The bug has been there since July 2024. The reason that it has gone unnoticed is that properties still have no wide adoption, and the bug triggers in very specific circumstances.

In the circumstances that it does trigger, it makes the IR non-roundtripable through bytecode (this is a basic feature, and one that it's usually tested in a lit suite), and it produces no error message making it hard to debug.

In terms of impact, without the fix, people might not be able to use OptionalProp. However, given that MLIR still doesn't recommend the usage of releases for dev (ie. we still recommend people to use main), that there are many unsolved issues around properties, and that a downstream can solve the issue without this fix. I would likely not included if it's too much hazzle.

Nonetheless, @yu810226 is there a reason you needed to backport this patch?

Thanks @fabianmcg for the explanation. Given what you describe I think we are not going to include this change in the upcoming 21.x release, and instead it should be available in the upcoming 22.x release.